fix(jans-cedarling): fix benchmarks to actually work #12923
Conversation
…hmark Signed-off-by: dagregi <[email protected]>
Signed-off-by: dagregi <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughBenchmarks and a benchmark-checking script were modified: request-builder functions were made non-fallible and now accept references/issuer URLs; a pre-benchmark JWT validation helper was added; issuer strings were changed; and the benchmark threshold logic was adjusted for specific problematic benchmarks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The added exclusion threshold should be removed once #12947 is fixed. Signed-off-by: dagregi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @jans-cedarling/scripts/check_benchmarks.py:
- Around line 38-41: Replace the if-else threshold selection with a lookup
mapping to make it easier to extend: create BENCHMARK_THRESHOLDS mapping
specific benchmark_name keys to EXCLUSION_THRESHOLD, then set threshold =
BENCHMARK_THRESHOLDS.get(benchmark_name, THRESHOLD_NS) instead of checking
PROBLEMATIC_BENCHMARKS; keep existing constants EXCLUSION_THRESHOLD,
THRESHOLD_NS and the benchmark_name symbol to preserve behavior.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jans-cedarling/scripts/check_benchmarks.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Run benchmarks using `cargo bench -p cedarling`
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Applies to jans-cedarling/**/*.rs : Review clippy.toml for project-specific lint rules
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cleanup
- GitHub Check: python_tests (3.10)
- GitHub Check: wasm_tests
- GitHub Check: rust_tests
- GitHub Check: python_tests (3.11)
- GitHub Check: golang_tests
- GitHub Check: rust_benchmarks
🔇 Additional comments (2)
jans-cedarling/scripts/check_benchmarks.py (2)
43-50: LGTM! Threshold variable used consistently.The threshold variable is correctly applied in the comparison and both log messages. The implementation ensures that the appropriate threshold is used for each benchmark.
10-16: Issue #12947 is open and properly tracked. The code change is a legitimate temporary measure.The 1.5ms threshold for these specific benchmarks is justified by a documented performance bottleneck (entity building and schema validation overhead in issue #12947), not an arbitrary workaround. The code comment correctly references the issue, which contains the explanation for why these benchmarks exceed the standard 1ms threshold during this optimization phase.
Prepare
Description
Target issue
closes #12900
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.